-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
errors, child_process: migrate to using internal/errors #11300
Conversation
c05e185
to
7839c5b
Compare
@nodejs/ctc ... can I please get a review on this? |
7839c5b
to
d3e735c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there
doc/api/errors.md
Outdated
an argument of the wrong type has been passed to a Node.js API. | ||
|
||
<a id="ERR_INVALID_HANDLE_TYPE"></a> | ||
###ERR_INVALID_HANDLE_TYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space
doc/api/errors.md
Outdated
### ERR_INVALID_OPT_VALUE | ||
|
||
The `'ERR_INVALID_OPT_VALUE'` error code is used generically to identify when | ||
an invalid or unexpected value has been passed on an options object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: passed in?
doc/api/errors.md
Outdated
### ERR_IPC_ONE_PIPE | ||
|
||
The `'ERR_IPC_ONE_PIPE'` error code is used when an attempt is made to create | ||
a child Node.js process using more than one IPC communications channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: communication
doc/api/errors.md
Outdated
## Node.js Error Codes | ||
|
||
<a id="ERR_CHANNEL_CLOSED"></a> | ||
### ERR_CHANNEL_CLOSED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe ERR_IPC_CHANNEL_CLOSED
?
doc/api/errors.md
Outdated
<a id="ERR_INVALID_HANDLE_TYPE"></a> | ||
###ERR_INVALID_HANDLE_TYPE | ||
|
||
The '`ERR_INVALID_HANDLE_TYPE`' error code is used when an attempt is made to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto. ERR_IPC_INVALID_HANDLE_TYPE
lib/internal/errors.js
Outdated
// Add new errors from here... | ||
|
||
function invalidArgType(name, expected, actual) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read it correctly, this would print The "options" argument must be type Object
. It looks better to me as The "options" argument must be of type Object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have some tests for this function OK, tests are in #11294
lib/internal/child_process.js
Outdated
@@ -772,11 +773,10 @@ function _validateStdio(stdio, sync) { | |||
case 'ignore': stdio = ['ignore', 'ignore', 'ignore']; break; | |||
case 'pipe': stdio = ['pipe', 'pipe', 'pipe']; break; | |||
case 'inherit': stdio = [0, 1, 2]; break; | |||
default: throw new TypeError('Incorrect value of stdio option: ' + stdio); | |||
default: throw new errors.TypeError('ERR_INVALID_OPT_VALUE', stdio); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name
argument is missing
lib/internal/child_process.js
Outdated
} | ||
} else if (!Array.isArray(stdio)) { | ||
throw new TypeError('Incorrect value of stdio option: ' + | ||
util.inspect(stdio)); | ||
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', util.inspect(stdio)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks ok to me, but needs updates requested by @targos
e08db62
to
9470c7f
Compare
9470c7f
to
0dd4e33
Compare
@@ -54,7 +61,6 @@ function message(key, args) { | |||
// Utility function for registering the error codes. Only used here. Exported | |||
// *only* to allow for testing. | |||
function E(sym, val) { | |||
assert(messages.has(sym) === false, `Error symbol: ${sym} was already used.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this check? (The test for it is removed in the second commit btw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of issues in the loading order of the errors.js
module relative to assert.js
on startup. There is a circular dependency that happens that causes the util.js
used within assert.js
to fail depending on when assert
is loaded. I could replace this with a simple throw rather than the call to assert if you'd be more comfortable with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with removing it. I was just curious. The risk of reusing the same code is low if we keep them together and in alphabetical order.
lib/internal/errors.js
Outdated
@@ -86,12 +86,32 @@ module.exports = exports = { | |||
// | |||
// Note: Please try to keep these in alphabetical order | |||
E('ERR_ASSERTION', (msg) => msg); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops! fixed
lib/internal/errors.js
Outdated
E('ERR_INVALID_ARG_TYPE', invalidArgType); | ||
E('ERR_INVALID_CALLBACK', 'callback must be a function'); | ||
E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed'); | ||
E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed'); | ||
E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type'); | ||
E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type'); | ||
E('ERR_IPC_CHANNEL_CLOSED', 'channel closed'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep the codes in alphabetical order relative to the existing ones.
@@ -125,12 +125,6 @@ assert.throws(() => { | |||
message: /^Error for testing 2/ })); | |||
}, /AssertionError: .+ does not match \S/); | |||
|
|||
assert.doesNotThrow(() => errors.E('TEST_ERROR_USED_SYMBOL')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please bundle this change with the same commit that removed the error
0dd4e33
to
d7a1a80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This needs a rebase.
d7a1a80
to
55be06e
Compare
Rebased. Ping @mhdawson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
55be06e
to
6f5a9ab
Compare
Thank you @mhdawson and @targos. Rebased again and new CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7715/ |
Use of assert must be lazy to allow errors to be used early before the process is completely set up
6f5a9ab
to
14cacc6
Compare
Use of assert must be lazy to allow errors to be used early before the process is completely set up PR-URL: #11300 Ref: #11273 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #11300 Ref: #11273 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Landed in f0b7025 and 7632761 |
Ref: #11273
Semver-major because error messages are changed.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
errors, child_process